Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/utils/bitwarden_resources.go (2)

26-32: Constants are duplicated with aws_resources.go.

The constants externalSecretsAPIVersionV1 and clusterSecretStoreKind are also defined in test/utils/aws_resources.go. Consider extracting these to a shared constants file within the utils package to avoid drift.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/utils/bitwarden_resources.go` around lines 26 - 32, The constants
externalSecretsAPIVersionV1 and clusterSecretStoreKind are duplicated across
bitwarden_resources.go and aws_resources.go; extract these into a shared utils
package constants file (e.g., constants.go) and replace the local definitions in
both bitwarden_resources.go and aws_resources.go with references to the new
shared constants (externalSecretsAPIVersionV1, clusterSecretStoreKind), removing
the duplicate declarations so both files import/use the single source of truth.

---

70-96: Consider handling SetNestedField errors.

While SetNestedField typically only fails if the path is structurally invalid (which won't happen with these compile-time known paths), explicitly checking the error would make the code more defensive.

-	_ = unstructured.SetNestedField(u.Object, []interface{}{
+	if err := unstructured.SetNestedField(u.Object, []interface{}{
 		map[string]interface{}{
 			"secretKey": "value",
 			"remoteRef": map[string]interface{}{
 				"key": remoteKey,
 			},
 		},
-	}, "spec", "data")
+	}, "spec", "data"); err != nil {
+		panic(fmt.Sprintf("failed to set spec.data: %v", err))
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/utils/bitwarden_resources.go` around lines 70 - 96, The SetNestedField
calls in BitwardenExternalSecretByName and BitwardenExternalSecretByUUID ignore
returned errors; update both functions to capture the error from
unstructured.SetNestedField and handle it (e.g., if err != nil {
panic(fmt.Errorf("failed to set spec.data for %s: %w", name, err)) }) so
failures are surfaced; reference the SetNestedField call sites in
BitwardenExternalSecretByName and BitwardenExternalSecretByUUID and include the
original error text in the panic/log message for diagnosability.

test/utils/bitwarden_api_runner.go (1)

35-38: Consider pinning the curl image tag for reproducibility.

Using docker.io/curlimages/curl:latest can lead to non-reproducible test behavior if the image is updated with breaking changes. Consider pinning to a specific version.

 const (
-	bitwardenAPITestRunnerImage = "docker.io/curlimages/curl:latest"
+	bitwardenAPITestRunnerImage = "docker.io/curlimages/curl:8.7.1"
 	bitwardenCredMountPath      = "/etc/bitwarden-cred"
 )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/utils/bitwarden_api_runner.go` around lines 35 - 38, The constant
bitwardenAPITestRunnerImage currently uses the mutable tag
"docker.io/curlimages/curl:latest" which can cause flaky tests; change the value
of bitwardenAPITestRunnerImage to a specific, immutable curlimages tag (e.g., a
known semver like :8.x.y) and add a short comment noting why the tag is pinned;
ensure any CI/test docs referencing this constant are updated when you
intentionally upgrade the pinned tag.

Makefile (2)

182-182: Add PHONY declaration for the targets on this line.

The static analysis tool flagged that targets listed here should be declared as .PHONY to ensure they're always executed regardless of whether a file with that name exists.

+.PHONY: fmt vet test test-unit test-e2e e2e-coverage-publish run update-vendor update-dep
 fmt vet test test-unit test-e2e e2e-coverage-publish run update-vendor update-dep: GOFLAGS=

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Makefile` at line 182, The Makefile line defining the combined targets "fmt
vet test test-unit test-e2e e2e-coverage-publish run update-vendor update-dep:
GOFLAGS=" lacks a .PHONY declaration, so add a .PHONY rule listing these exact
target names (fmt vet test test-unit test-e2e e2e-coverage-publish run
update-vendor update-dep) elsewhere in the Makefile to ensure they always run
regardless of files with those names; update the .PHONY block to include all
these targets.

---

219-222: Codecov uploader pinning and SHA256 verification are correct.

The implementation correctly pins v0.7.6 and verifies the SHA256 checksum before execution. v0.7.6 is a valid release. However, v0.8.0 is now the latest available version. If this version is not pinned for specific compatibility reasons, consider upgrading to v0.8.0 to benefit from any bug fixes or security patches.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Makefile` around lines 219 - 222, Update the pinned Codecov uploader by
changing the CODECOV_VERSION variable from v0.7.6 to v0.8.0 and adjust the
derived URLs (CODECOV_URL and CODECOV_SHA_URL) will automatically follow; if
there's a deliberate compatibility constraint, add a brief comment next to
CODECOV_VERSION explaining why v0.7.6 must remain pinned instead of upgrading to
v0.8.0.

test/utils/bitwarden.go (2)

182-201: Consider validating organization and project IDs.

FetchBitwardenCredsFromSecret validates that token is non-empty but silently returns empty strings for orgID and projectID if they're missing. If these are required for Bitwarden API operations, consider validating them as well.

 	if v, ok := secret.Data[BitwardenCredKeyOrgID]; ok {
 		orgID = string(v)
+	} else {
+		return "", "", "", fmt.Errorf("bitwarden creds secret %s/%s missing required key %q", secretNamespace, secretName, BitwardenCredKeyOrgID)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/utils/bitwarden.go` around lines 182 - 201,
FetchBitwardenCredsFromSecret currently only enforces token presence but
silently allows orgID and projectID to be empty; update the function to validate
BitwardenCredKeyOrgID and BitwardenCredKeyProjectID like TokenSecretKey and
return a descriptive error if either is missing or empty (use the same error
formatting pattern as the existing token check, referencing
secretNamespace/secretName and the missing key names) so callers receive a clear
failure when required organization or project IDs are absent.

---

326-341: Permissive error handling may mask connectivity issues.

The reachability check treats http_code=000 (connection refused/timeout/TLS failure) and empty logs as success to avoid failing the suite. While this is pragmatic, it could mask real connectivity problems that would cause subsequent tests to fail with less informative errors.

Consider at minimum logging a warning when these permissive paths are taken, so operators can investigate if tests fail later.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/utils/bitwarden.go` around lines 326 - 341, In the PodFailed case of the
reachability check (the switch handling corev1.PodFailed), add a warning log
before returning true on the permissive paths so connectivity issues aren't
silently ignored: when getPodLogs(...) contains "http_code=000" or when logs
contain "error on the server" or are empty/whitespace, call the test/logger
warning function to record the pod/container identity (use
formatPodContainerStatus(p) and podName/BitwardenOperandNamespace) and the raw
logs, then return true,nil as before.

test/utils/dynamic_resources.go (1)

112-127: Input mutation may surprise callers.

getResourceInterface mutates the input unstructuredObj by calling SetNamespace (Line 121). This side effect could be unexpected for callers who pass an object they intend to reuse. Consider documenting this behavior or working with a copy.

 func (d DynamicResourceLoader) getResourceInterface(unstructuredObj *unstructured.Unstructured, overrideNamespace string) dynamic.ResourceInterface {
 	gvk := unstructuredObj.GroupVersionKind()
 	gr, err := restmapper.GetAPIGroupResources(d.KubeClient.Discovery())
 	require.NoError(d.t, err)
 	mapper := restmapper.NewDiscoveryRESTMapper(gr)
 	mapping, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version)
 	require.NoError(d.t, err)
 	if mapping.Scope.Name() == meta.RESTScopeNameNamespace {
 		if overrideNamespace != "" {
-			unstructuredObj.SetNamespace(overrideNamespace)
+			unstructuredObj.SetNamespace(overrideNamespace) // Note: mutates input object
 		}
 		require.NotEmpty(d.t, unstructuredObj.GetNamespace(), "Namespace can not be empty for namespaced resource")
 		return d.DynamicClient.Resource(mapping.Resource).Namespace(unstructuredObj.GetNamespace())
 	}
 	return d.DynamicClient.Resource(mapping.Resource)
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/utils/dynamic_resources.go` around lines 112 - 127, The
getResourceInterface function currently mutates the caller's unstructuredObj by
calling SetNamespace on it when applying overrideNamespace; avoid surprising
side-effects by operating on a copy: create a deep copy of unstructuredObj
(e.g., via unstructuredObj.DeepCopy()), set the namespace on the copy rather
than the original, and use the copy's GetNamespace when selecting the Resource
Interface; alternatively, clearly document that getResourceInterface will mutate
unstructuredObj if overrideNamespace is provided. Ensure references to
SetNamespace, unstructuredObj, overrideNamespace and getResourceInterface are
updated accordingly.

test/utils/artifact_dump.go (1)

171-177: Consider pre-compiling the regex for sanitizeFilename.

The regex is compiled on every call. While acceptable for e2e test utilities where this runs infrequently, pre-compiling at package level would be slightly more efficient.

♻️ Optional: Pre-compile regex

+var sanitizeFilenameRe = regexp.MustCompile(`[^a-zA-Z0-9_-]`)
+
 func sanitizeFilename(s string) string {
-	s = regexp.MustCompile(`[^a-zA-Z0-9_-]`).ReplaceAllString(s, "_")
+	s = sanitizeFilenameRe.ReplaceAllString(s, "_")
 	if len(s) > 100 {
 		s = s[:100]
 	}
 	return s
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/utils/artifact_dump.go` around lines 171 - 177, The sanitizeFilename
function currently compiles the regex on every call; move the
regexp.MustCompile(`[^a-zA-Z0-9_-]`) to a package-level variable (e.g.,
filenameSanitizeRegex) and update sanitizeFilename to use
filenameSanitizeRegex.ReplaceAllString(s, "_") so the regex is compiled once and
behavior (truncation to 100 chars) remains unchanged.

test/e2e/bitwarden_api_test.go (1)

139-144: Consider documenting why both 200 and 400 are acceptable.

The test accepts either HTTP 200 or 400 for empty ids list. Adding a brief comment explaining the expected API behavior variance would improve clarity.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/bitwarden_api_test.go` around lines 139 - 144, Add a short comment
above the It("GET /rest/api/1/secrets-by-ids with empty ids should return 200 or
400", ...) test explaining why both HTTP 200 and 400 are accepted (e.g.,
different implementations may treat an empty ids list as a valid no-op returning
200 or as a client error returning 400), so future readers of the test (and
maintainers of runAPIJob/script and the API contract) understand the intended
variance; reference the test name, the script variable that posts '{"ids":[]}',
and runAPIJob to locate the exact spot to insert the comment.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes